feat: added API to get subscribers of a thread. #415
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #415 +/- ##
==========================================
+ Coverage 96.10% 96.14% +0.04%
==========================================
Files 58 58
Lines 4514 4562 +48
==========================================
+ Hits 4338 4386 +48
Misses 176 176
☔ View full report in Codecov by Sentry. |
AhtishamShahid
force-pushed
the
ahtisham/INF-943
branch
from
53c38c5 to
1ac14a1
Compare
| query[:subscriber_id] = params[:subscriber_id] if params[:subscriber_id] | ||
| query[:source_id] = thread_id | ||
| query[:source_type] = params[:source_type] if params[:source_type] |
There was a problem hiding this comment.
I think we only need the source_id, no?
There was a problem hiding this comment.
added subscriber_id and source type to provide filtering of all available attributes. They are not used yet
There was a problem hiding this comment.
From what I understand, here the source type should always be that of a thread, otherwise this will be a general purpose API for subscriptions, but this seems to be focussed on threads.
If that is the case, you should only have a one subscription between one use and thread, so specifying a subscriber_id might not make sense.
| content_type :json | ||
|
|
||
| { | ||
| collection: subscriptions.map(&:to_hash), |
There was a problem hiding this comment.
do we need to return the whole collection, or only the ids?
There was a problem hiding this comment.
subscription object has only 4 attributes, so I guess we can return all without taking any performance hit.
AhtishamShahid
changed the title
|
@xitij2000 it would be great if you can take a quick look. |
| @@ -28,3 +28,26 @@ | |||
| delete "#{APIPREFIX}/users/:user_id/subscriptions" do |user_id| | |||
| user.unsubscribe(source).to_hash.to_json | |||
| end | |||
|
|
|||
| get "#{APIPREFIX}/subscriptions/:thread_id" do |thread_id| | |||
There was a problem hiding this comment.
I think it would be better to follow the pattern above and put this under threads.
i.e.
| get "#{APIPREFIX}/subscriptions/:thread_id" do |thread_id| | |
| get "#{APIPREFIX}/threads/:thread_id/subscriptions" do |thread_id| |
That way it is not ambiguous what we are managing. As mentioned elsewhere, if in the future we need subscriptions for a different resource, it will be hard to disambiguate between the ids.
| query[:subscriber_id] = params[:subscriber_id] if params[:subscriber_id] | ||
| query[:source_id] = thread_id | ||
| query[:source_type] = params[:source_type] if params[:source_type] |
There was a problem hiding this comment.
From what I understand, here the source type should always be that of a thread, otherwise this will be a general purpose API for subscriptions, but this seems to be focussed on threads.
If that is the case, you should only have a one subscription between one use and thread, so specifying a subscriber_id might not make sense.
|
Is this PR stale? |
Ticket
https://2u-internal.atlassian.net/browse/INF-943
added subscribers in thread object